Skip to content

Conversation

@pjfanning
Copy link
Contributor

Based on POC in #5138

This test seems to indicate that the OTel Span is shared and that Pekko actor operates in same span as the Pekko HTTP route.

I may have made a mistake in the test. If this test looks ok, it might be useful to merge it as a regression test.

@pjfanning pjfanning requested a review from a team as a code owner October 3, 2024 20:50
import io.opentelemetry.testing.internal.armeria.common.HttpHeaderNames;
import java.time.Duration;

public final class PekkoHttpTestSetup {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you consider extending AbstractHttpServerUsingTest instead of introducing this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractHttpServerUsingTest carries a load of existing tests with it. I just want to add one new test that is independent of AbstractHttpServerUsingTest's existing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WebClient setup in this class is copied from private code in this lib. If I am allowed to modify that code so that I can access then I can refactor or remove this new class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are mixing AbstractHttpServerUsingTest with AbstractHttpServerTest. AbstractHttpServerUsingTest contains zero tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I need this new class is because I need a WebClient instance. AbstractHttpServerUsingTest does not create a WebClient.

HttpServerInstrumentationExtension creates a WebClient but it needs an InstrumentationTestRunner.

These are all major rabbit holes.

I can get rid of this class and just create my own WebClient in my test class. I just didn't want to write this in Scala but I will if this Java class is a problem for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically you'd extend the AbstractHttpServerUsingTest and in the test register the extension with

@RegisterExtension
static final InstrumentationExtension testing = HttpServerInstrumentationExtension.forAgent();

Then you can use the WebClient in your test. You can also use testing.runWithSpan(...) to create spans and testing.waitAndAssertTraces(...) to verify that the spans have the correct structure instead of trying to encode trace and span info in the http response. I'd suggest you give it a try, it will be much nicer than what you currently have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - I'll abandon this

I don't think it is healthy to have such a high bar for tests.

@pjfanning pjfanning closed this Oct 8, 2024
@pjfanning pjfanning deleted the pekko-http-actor branch October 8, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants